Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(CI_Pipeline): resolve dependency issues causing build failures #1508

Merged
merged 9 commits into from
Jan 9, 2025

Conversation

ArslanSaleem
Copy link
Collaborator

@ArslanSaleem ArslanSaleem commented Jan 9, 2025

Important

This PR restructures CI workflows, updates dependencies, and fixes import order issues to resolve build failures.

  • CI Workflows:
    • Add new ci-core.yml for core tests on multiple OS and Python versions.
    • Rename ci.yml to ci-extensions.yml and update job names.
  • Dependencies:
    • Update pandasai dependency to ^3.0.0a0 in bigquery/pyproject.toml.
  • Code Quality:
    • Fix import order in __init__.py, config.py, and request.py.
  • Testing:
    • Remove unused samples in test_convert_df_to_yml() in test_dataframe_serializer.py.

This description was created by Ellipsis for 234c053. It will automatically update as commits are pushed.

@ArslanSaleem ArslanSaleem requested a review from gventuri January 9, 2025 08:56
@dosubot dosubot bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Jan 9, 2025
Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Reviewed everything up to 3e6df5f in 11 seconds

More details
  • Looked at 13 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. extensions/ee/connectors/bigquery/pyproject.toml:10
  • Draft comment:
    Switching to an alpha version pandasai = "^3.0.0a0" might introduce instability. Ensure this is intentional and tested.
  • Reason this comment was not posted:
    Comment did not seem useful.

Workflow ID: wflow_MEuQJa26wBTQOQxy


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. and removed size:XS This PR changes 0-9 lines, ignoring generated files. labels Jan 9, 2025
Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Incremental review on b470344 in 21 seconds

More details
  • Looked at 61 lines of code in 4 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. pandasai/__init__.py:19
  • Draft comment:
    The import order was corrected to follow PEP 8 guidelines, which is a good practice.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The import order was changed to follow PEP 8 guidelines, which is a good practice.
2. pandasai/config.py:6
  • Draft comment:
    The import order was corrected to follow PEP 8 guidelines, which is a good practice.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The import order was changed to follow PEP 8 guidelines, which is a good practice.
3. pandasai/helpers/request.py:10
  • Draft comment:
    The import statement for load_dotenv was moved to follow PEP 8 guidelines, which is a good practice.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The import statement for load_dotenv was moved to follow PEP 8 guidelines, which is a good practice.

Workflow ID: wflow_dw25Rdm3WCdvHhzT


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Incremental review on 972ccd1 in 9 seconds

More details
  • Looked at 14 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_uqvgBX5f8v8snDcm


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skipped PR review on 170c0f9 because no changed files had a supported extension. If you think this was in error, please contact us and we'll fix it right away.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Incremental review on 6d5ddfa in 11 seconds

More details
  • Looked at 20 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. .github/workflows/ci.yml:19
  • Draft comment:
    Using 'sudo' might not be necessary in all environments and could fail if the runner lacks sudo privileges. Ensure the runner has the necessary permissions or handle potential failures gracefully.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The cleanup step is being executed for non-Windows environments, which is fine. However, the use of 'sudo' might not be necessary in all environments, and it could fail if the runner does not have sudo privileges. It's better to ensure that the runner has the necessary permissions or handle the failure gracefully.
2. .github/workflows/ci.yml:17
  • Draft comment:
    The phrase 'Clean up instance space' is grammatically correct, but could be more descriptive. Consider 'Clean up disk space on instance' for clarity.
  • Reason this comment was not posted:
    Confidence changes required: 33%
    The phrase 'Clean up instance space' is grammatically correct, but it could be more descriptive. However, it is not a significant issue.

Workflow ID: wflow_1PRc9lwSMU6HpnxD


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Jan 9, 2025
Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Incremental review on 71c94a5 in 28 seconds

More details
  • Looked at 347 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. .github/workflows/ci-core.yml:51
  • Draft comment:
    The installation of the 'future' package seems unnecessary as it is not used later in the script. Consider removing this step to streamline the workflow.
  • Reason this comment was not posted:
    Comment did not seem useful.
2. .github/workflows/ci-extensions.yml:68
  • Draft comment:
    Consider rephrasing for clarity and consistency:
              Write-Host "Executing tests for $($_.FullName)"
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The error message in the script is clear and concise, but the use of 'Running tests for' could be improved for better clarity and consistency.

Workflow ID: wflow_bLYz6mrrx8B3RULE


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link

codecov bot commented Jan 9, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.62%. Comparing base (63e8c26) to head (234c053).
Report is 104 commits behind head on release/v3.

❗ There is a different number of reports uploaded between BASE (63e8c26) and HEAD (234c053). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (63e8c26) HEAD (234c053)
1 0
Additional details and impacted files
@@              Coverage Diff               @@
##           release/v3    #1508      +/-   ##
==============================================
- Coverage       79.09%   72.62%   -6.47%     
==============================================
  Files             149       64      -85     
  Lines            6013     2214    -3799     
==============================================
- Hits             4756     1608    -3148     
+ Misses           1257      606     -651     
Flag Coverage Δ
unittests 72.62% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Changes requested. Incremental review on 4669f39 in 1 minute and 0 seconds

More details
  • Looked at 113 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 8 drafted comments based on config settings.
1. .github/workflows/ci-extensions.yml:57
  • Draft comment:
    Consider rephrasing for consistency and clarity:
              echo "Installing dependencies for $dir..."
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The suggestion is purely stylistic and doesn't affect functionality. Looking at lines 62, 75, 80, 93, 98, 113, 125, and 137, none of the other similar log messages use ellipses. Adding one here would actually make this message inconsistent with the others. The current format is clear and concise.
    Perhaps adding ellipses could make it clearer that this is an ongoing process that will take some time?
    The ongoing nature of the process is already clear from the context and structure of the script. Adding ellipses would create inconsistency with other similar messages in the file.
    This comment should be deleted as it suggests a purely stylistic change that would actually reduce consistency with other similar messages in the file.
2. .github/workflows/ci-extensions.yml:62
  • Draft comment:
    Consider rephrasing for consistency and clarity:
              echo "Running tests for $dir..."
  • Reason this comment was not posted:
    Marked as duplicate.
3. .github/workflows/ci-extensions.yml:75
  • Draft comment:
    Consider rephrasing for consistency and clarity:
              echo "Installing dependencies for $dir..."
  • Reason this comment was not posted:
    Marked as duplicate.
4. .github/workflows/ci-extensions.yml:80
  • Draft comment:
    Consider rephrasing for consistency and clarity:
              echo "Running tests for $dir..."
  • Reason this comment was not posted:
    Marked as duplicate.
5. .github/workflows/ci-extensions.yml:93
  • Draft comment:
    Consider rephrasing for consistency and clarity:
              echo "Installing dependencies for $dir..."
  • Reason this comment was not posted:
    Marked as duplicate.
6. .github/workflows/ci-extensions.yml:98
  • Draft comment:
    Consider rephrasing for consistency and clarity:
              echo "Running tests for $dir..."
  • Reason this comment was not posted:
    Marked as duplicate.
7. .github/workflows/ci-extensions.yml:113
  • Draft comment:
    Consider rephrasing for consistency and clarity:
              Write-Host "Running tests for $($_.FullName)..."
  • Reason this comment was not posted:
    Marked as duplicate.
8. .github/workflows/ci-extensions.yml:125
  • Draft comment:
    Consider rephrasing for consistency and clarity:
              Write-Host "Running tests for $($_.FullName)..."
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_in34A1ztgEsjVcFt


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@@ -137,11 +140,15 @@ jobs:
Pop-Location
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test execution step is missing for enterprise extensions on Windows. Add poetry run pytest tests/ after installing dependencies.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Changes requested. Incremental review on 098d685 in 51 seconds

More details
  • Looked at 50 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. .github/workflows/ci-extensions.yml:162
  • Draft comment:
    Switching from Poetry to pip for installing coverage can lead to inconsistencies. Ensure this change is intentional and won't cause issues with dependency management.
  • Reason this comment was not posted:
    Comment did not seem useful.
2. .github/workflows/ci-extensions.yml:67
  • Draft comment:
    The phrase 'Removing envs' should be 'Removing environments' for clarity and correctness. This applies to lines 90 and 113 as well.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    This is a very minor suggestion about log message wording. "envs" is a commonly used shorthand in the context of Python virtual environments, especially with tools like Poetry. The message is clear enough for developers who will be reading CI logs. This seems like an unnecessarily pedantic change that doesn't improve code quality or clarity.
    The comment does point out a consistency issue in terminology, and "environments" is more formal and clearer for newcomers.
    While more formal, changing CI log messages from a common technical shorthand to a longer form doesn't provide meaningful value and goes against the rule about not making purely informative comments or obvious suggestions.
    Delete this comment as it suggests a trivial change to log messages that doesn't improve code quality or fix any real issues.

Workflow ID: wflow_50dVBZ5EYAbCgfQz


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

.github/workflows/ci-extensions.yml Outdated Show resolved Hide resolved
Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Incremental review on 234c053 in 33 seconds

More details
  • Looked at 31 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. .github/workflows/ci-extensions.yml:70
  • Draft comment:
    Ensure that the --all flag is supported in the version of Poetry being used, as this change relies on it.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The change from using poetry env list --all | awk '{print $1}' | xargs -n 1 poetry env remove to poetry env remove --all is a simplification and should work as intended. However, it's important to ensure that the --all flag is supported in the version of Poetry being used.
2. .github/workflows/ci-extensions.yml:67
  • Draft comment:
    Consider changing 'Removing envs' to 'Removing environments' for clarity and grammatical correctness. This applies to lines 67, 90, and 113.
  • Reason this comment was not posted:
    Comment was on unchanged code.

Workflow ID: wflow_ZhSvHrjcBI85r4xZ


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@gventuri gventuri merged commit c9857e2 into release/v3 Jan 9, 2025
13 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants